-
Notifications
You must be signed in to change notification settings - Fork 809
Backchannel Logout #1573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Backchannel Logout #1573
Conversation
Before going further with this PR and working on tests/documentation, could please someone comment about the general structure of the implementation? My main concerns:
|
|
|
36a433e
to
4aeac81
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1573 +/- ##
==========================================
- Coverage 97.38% 96.06% -1.32%
==========================================
Files 34 35 +1
Lines 2214 2315 +101
==========================================
+ Hits 2156 2224 +68
- Misses 58 91 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4aeac81
to
8750834
Compare
|
b89d426
to
d3c2684
Compare
@lullis it looks like the next step is expanding test coverage right now the patch is just shy of 70%, our target is 100% coverage on patches going forward. At the very least we aim to match the current total coverage. |
@dopry this coverage report seems to be stale and based on a previous commit. How can I re-run the coverage report? |
It seems something has changed with the jazzband Codecov account and our codecov uploads are getting rate limited. I don't have the necessary permissions to add a the codecov repository upload token to the repo secrets and update our CI to use it. We're working on transferring out of jazzband right now to get away from those types of restrictions. In the meantime I'll keep trying to rerun the build to get coverage uploaded. |
I will take a look to see if I can run the report of the diff locally. Running coverage for the whole report showed that all files touched were at least at 99% coverage. |
Hi @dopry, is there any update? |
No @jezdez still hasn't transferred the repo. |
@lullis org is transferred. Can you rebase this PR on the latest master? |
73f43ce
to
4e37550
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
@lullis it looks like we still have some pretty big gaps in test code coverage for the backchannel logout functionality. Will you have time to work on that? Is there another of your PRs that you would want to prioritize? |
a58ff7f
to
780a113
Compare
@dopry , I've increased the test coverage as much as I could, but codecov is hung up on one line on application/models.py not being covered. Any tips on the best approach to test that exceptions are being caught in the middle of a function, or is it okay as it is? |
780a113
to
81d83ec
Compare
81d83ec
to
6b49a3d
Compare
@lullis I would mock
before exercising Here are some links that might shed some light on how to test exceptions. https://docs.python.org/3/library/unittest.mock.html |
1801a4b
to
f537328
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're pretty close to an initial implementation. There are some organizational issues I'd like to see addressed and I'm on the fence about whether it is wise to include OIDC_BACKCHANNEL_LOGOUT_HANDLER or drop it for now until we get some firm use cases for it.
oauth2_provider/models.py
Outdated
@@ -629,6 +636,53 @@ def revoke(self): | |||
""" | |||
self.delete() | |||
|
|||
def send_backchannel_logout_request(self, ttl=timedelta(minutes=10)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why put the send_backchannel_logout_request
method on AbstractIdToken? It doesn't operate on the token and the token is not an entity I think of as being responsible for sending requests. I feel like it's responsibility it carrying the identity data. A standalone function in handlers.py that receives an id_token as an argument would be more straight forward approach and would keep the handler bundled with where it is used for easier discoverability if someone were to want to extend it's scope by wrapping it in a proxy function and overriding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original implementation had a LogoutToken model for this, and I ended up moving to IDToken because most of the data needed is there. I am not a fan of putting too much business logic in a handlers module, but I can move it there if you prefer it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also not a fan of putting too much business logic in a handler module. I recognize much of the data we need it on the AbstractIdToken, but the method doesn't really depend (like a dependency injected at construction time) on it as much as requires the token as parameter at call time. I'm also not a fan of handler functions floating around in the models.py such that importing them into a handler module required pulling in the entirety of models and I've have bad experiences with circular dependencies when I do that and need to defer the import to inside the function. Maybe there is another more appropriate place if we want to make it common and shared... the first that comes to mind is oauth2_backends.OAuthLibCore, which can already be overridden via oauth2_settings.OAUTH2_BACKEND_CLASS... or maybe backends.OAuth2Backend... Give it some thought and I'll try to do the same over the next couple days.
errors = [] | ||
|
||
if oauth2_settings.OIDC_BACKCHANNEL_LOGOUT_ENABLED: | ||
if not callable(oauth2_settings.OIDC_BACKCHANNEL_LOGOUT_HANDLER): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OIDC_BACKCHANNEL_LOGOUT_HANDLER is not documented. What are the use cases for making a setting to override the logout handler? Is the specification flexible enough that it makes sense to give people the option to override this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main use-case for a configurable handler is simple: I would like to give consumers of this library a hook to let them execute this function outside of the request cycle. For example, if someone wants to run this as a celery task or implement a function that can keep track of errors. I will add it to the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delegating to celery sounds reasonable, especially if people want to implement retries etc. Do we not have logging for error tracking or are you thinking of some particular 3rd party instrumentation? Can you expand on your thoughts in regard to error tracking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed some changes, you will see that now the handler is made to work on id_tokens, not on the user directly.
My idea was to leave it completely open to the developer using DOT. The way I see it, someone that wants to have the requests on celery could do something like this:
- On myapp/tasks.py
from celery import shared_task
from oauth2_provider.models import get_id_token_model
from oauth2_provider.exceptions import BackchannelLogoutRequestError
from oauth2_provider.handlers import send_backchannel_logout_request as dot_request
IDToken = get_id_token_model()
@shared_task
def send_backchannel_logout_request(id_token_id):
try:
id_token = IDToken.objects.get(id=id_token_id)
dot_request(id_token=id_token)
except IDToken.DoesNotExist:
handle_missing_id_token()
except BackchannelLogoutRequestError:
handle_error()
def schedule_logout_request(id_token):
send_backchannel_logout_request.delay(id_token.id)
- on myproject/settings.py
OAUTH2_PROVIDER = {
"OIDC_BACKCHANNEL_LOGOUT_ENABLED": True,
"OIDC_BACKCHANNEL_LOGOUT_HANDLER": "myapp.tasks.schedule_logout_request"
}
- Add migration to add backchannel logout support - Add parameters related to backchannel logout on OIDC Discovery View - Change application creation and update form - Change template that renders information about the application
6afbd7d
to
728c0a0
Compare
Fixes #1545
Description of the Change
Checklist
CHANGELOG.md
updated (only for user relevant changes)AUTHORS